@alexs-101 thanks for the PR! Before diving into the code, I wanted to make sure I understood the motivation behind each feature. Btw - it makes it a lot easier to review if things are split up into multiple PRs

Things that make sense to me

• force_extraction_of_lineage_from_custom_sql_queries - agree that tableau's lineage is pretty bad, and we should prefer our own to theirs. Likely will want to flip the default on this in the future
• The salesforce dialect hack - seems reasonable

Things I'd like to discuss

• The TSQLv2 dialect - ideally we contribute this back to sqlglot, or if they're not willing to accept it, we can merge it into the acryl-sqlglot fork
• disable_schema_awarenes_during_parsing_of_sql_queries and the schema_aware flag - I didn't realize that having custom SQL queries digress from the underlying table schemas was a common thing.
  • My goal in building the initial parser was that it would be permissive enough that it could handle missing columns gracefully. Do you have some specific examples where a certain query + schema info would fail, but just the query with no schema info generates results that are more correct? If so, maybe we can fix those instances, and avoid needing disabling schema awareness across the board
  • Also, you can also disable schema-awareness by not passing a graph instance to the SchemaResolver - that feels like a cleaner approach
• Adding in_tables_schemas - it seems like this can be fully derived from the rest of the SqlParsingResult? The logic might be better suited to live in a helper method, and not part of the core SqlParsingResult

One other thing - it seems like the t-sql changes only make sense when QUOTED_IDENTIFIER is OFF (as per https://stackoverflow.com/questions/62917351/what-is-the-syntax-of-a-string-literal-in-t-sql) - so I'm not sure it makes sense to change the default. It might make sense to make this configurable in some way

@hsheth2 big thanx for quick feedback! I will answer (or fix) everything tomorrow morning with a fresh mind.

tbh the fact that we have ingest_tables_external at all feels pretty strange to me - I guess it makes sense for CSVs and things, but for snowflake/other warehouse tables, we really should just rely on those other sources. But I guess it's alright for now, especially as it's disabled by default

it's a very helpful feature for those who has ability to ingest Tableau only and don't have access to other metadata sources. cold we keep it?

especially as it's disabled by default

yep

@hsheth2 finished, could you start tests again, please?

@hsheth2

Seems likt this test failed

test_create_list_get_ingestion_execution_request

E         - urn:li:dataHubExecutionRequest:219530d5-775d-4a38-9c93-d393cc8239a4
E         + urn:li:dataHubExecutionRequest:af738e1d-4870-4519-af1a-fc88c1d57d6b

I checked, it can not be due to my changes

@hsheth2 i pulled in latest changes from master, could you please start tests again?

@hsheth2 I checked failed tests and all of them look like failed NOT due to changes brought by the PR

FAILED tests/integration/s3/test_s3.py::test_data_lake_s3_ingest[multiple_files.json] - Failed: Metadata files differ (use `pytest --update-golden-files` to update):

Urn changed, urn:li:dataset:(urn:li:dataPlatform:s3,test-platform-instance.my-test-bucket/folder_a/folder_aa/folder_aaa/NPS.7.1.package_data_NPS.6.1_ARCN_Lakes_ChemistryData_v1_csv.csv,DEV):
<schemaMetadata> changed:
	Item aspect['fields'][20]['type']['type']['com.linkedin.schema.DateType'] added to dictionary.
	Item aspect['fields'][20]['type']['type']['com.linkedin.schema.StringType'] removed from dictionary.
	Value of aspect['fields'][20]['nativeDataType'] changed from "string" to "date".

FAILED tests/integration/s3/test_s3.py::test_data_lake_local_ingest[multiple_files.json] - Failed: Metadata files differ (use `pytest --update-golden-files` to update):

Urn changed, urn:li:dataset:(urn:li:dataPlatform:file,test-platform-instance.tests/integration/s3/test_data/local_system/folder_a/folder_aa/folder_aaa/NPS.7.1.package_data_NPS.6.1_ARCN_Lakes_ChemistryData_v1_csv.csv,DEV):
<schemaMetadata> changed:
	Item aspect['fields'][20]['type']['type']['com.linkedin.schema.DateType'] added to dictionary.
	Item aspect['fields'][20]['type']['type']['com.linkedin.schema.StringType'] removed from dictionary.
	Value of aspect['fields'][20]['nativeDataType'] changed from "string" to "date".

I will fix it, if you say - fix it. :)

@hsheth2 I checked failed tests and all of them look like failed NOT due to changes brought by the PR

........

I will fix it, if you say - fix it. :)

+1 They also fail in my PR #9874 as well

@alexs-101 @egemenberk I agree those failures look unrelated, and I believe have been fixed on the master branch. I just synced this branch with master, and hopefully that unblocks CI

found bug #10357 when the force_extraction_of_lineage_from_custom_sql_queries property is not leveraged and the error is thrown